kvserver: disable latch assertions further down #158143
Conversation
This commit moves disabling of the latch assertions from the latch declaration phase to the body of the corresponding commands. This serves two purposes: 1. Disabling of the assertions is now more targeted. It is per command rather than per batch. 2. The disabling behaviour is ephemeral, so we can remove the bool in SpanSet that doesn't belong there. It's a data structure that should have no opinion on whether the checks are enabled. Epic: none Release note: none
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
|
@iskettaneh I tested locally, with |
| // undeclared access to spans. This is generally set by requests that rely on | ||
| // other forms of synchronization for correctness (e.g. GCRequest). |
There was a problem hiding this comment.
"rely on other forms of synchronization" looks like a useful note, need to retain it in some form
| // | ||
| // See call to `AssertAllowed()` in GetGCThreshold() to understand why we need | ||
| // to disable these assertions for export requests. | ||
| reader = spanset.DisableReaderAssertions(reader) |
There was a problem hiding this comment.
The problem is, we probably need to retain some control of which assertions are we disabling. For now, DisableReaderAssertions will unwrap the SpanSetBatch, which will disable all assertions really. Now, we only have one assertion type (Latch assertions), but we are soon planning to add a new assertion (Forbidden spans)
3 participants
This commit moves disabling of the latch assertions from the latch declaration phase to the body of the corresponding commands. This serves two purposes:
SpanSetthat doesn't belong there. It's a data structure that should have no opinion on whether the checks are enabled.Epic: none
Release note: none